fix(audit): remediate system audit findings (P0, P1, P2)#1198
fix(audit): remediate system audit findings (P0, P1, P2)#1198BigSimmo wants to merge 25 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe PR adds standalone heavy-run lock bypassing and faster contention failures, exempts medical-term queries from unsupported-search short-circuiting, revokes selected database privileges, and adds a secure function for invoking the ingestion worker dynamically. ChangesHeavy run lock behavior
Medical-term search guard
Database access controls
Dynamic ingestion worker invocation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant invoke_ingestion_worker
participant Vault
participant net_http_post
Caller->>invoke_ingestion_worker: invoke with p_limit
invoke_ingestion_worker->>Vault: load cron_ingestion_jwt
Vault-->>invoke_ingestion_worker: return JWT
invoke_ingestion_worker->>net_http_post: POST worker request with bounded limit
net_http_post-->>invoke_ingestion_worker: return request id
invoke_ingestion_worker-->>Caller: return bigint request id
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/rag/rag-query-guard.ts`:
- Around line 19-23: Update shouldShortCircuitUnsupportedSearch so the existing
clearlyOutsideCorpusMedicalPattern exclusion checks run before the
MEDICAL_TERMS_REGEX bypass, ensuring explicit outside-corpus matches such as
“adolescent depression” and “SSRI” still short-circuit. Preserve the
medical-term bypass for queries that do not match the hard exclusion rules, and
add regression coverage for this precedence.
In `@supabase/migrations/20260724163951_revoke_api_grants.sql`:
- Around line 29-38: Update the comment above the revoke statements to state
that execution is being revoked for nine internal maintenance RPCs, matching the
nine functions listed below.
In `@supabase/migrations/20260724164600_dynamic_ingestion_worker_url.sql`:
- Around line 1-40: Lock down execute privileges for
public.invoke_ingestion_worker immediately after its definition by revoking
EXECUTE from PUBLIC, anon, and authenticated, matching the permission pattern
used by related SECURITY DEFINER functions. Preserve access only for explicitly
authorized roles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fce956f-7a25-46a9-951d-35078e1963b1
📒 Files selected for processing (4)
scripts/test-run-lock.mjssrc/lib/rag/rag-query-guard.tssupabase/migrations/20260724163951_revoke_api_grants.sqlsupabase/migrations/20260724164600_dynamic_ingestion_worker_url.sql
| export const MEDICAL_TERMS_REGEX = | ||
| /\b(?:bipolar|lithium|toxicity|clozapine|schizophrenia|depression|anorexia|anxiety|ssri|ssnri|psychosis|dosing|titration|olanzapine|quetiapine|risperidone|aripiprazole|haloperidol|valproate|lamotrigine|carbamazepine|cbt|ect)\b/i; | ||
|
|
||
| export function shouldShortCircuitUnsupportedSearch(query: string, analysis: ClinicalQueryAnalysis) { | ||
| if (MEDICAL_TERMS_REGEX.test(query)) return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not let the medical-term bypass override explicit exclusion rules.
MEDICAL_TERMS_REGEX overlaps clearlyOutsideCorpusMedicalPattern (depression and ssri). Because this return runs first, queries such as adolescent depression or SSRI skip the existing outside-corpus short circuit and proceed to retrieval. Move the hard exclusion checks before this bypass, or remove the overlapping terms and add regression tests that define the intended precedence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/rag/rag-query-guard.ts` around lines 19 - 23, Update
shouldShortCircuitUnsupportedSearch so the existing
clearlyOutsideCorpusMedicalPattern exclusion checks run before the
MEDICAL_TERMS_REGEX bypass, ensuring explicit outside-corpus matches such as
“adolescent depression” and “SSRI” still short-circuit. Preserve the
medical-term bypass for queries that do not match the hard exclusion rules, and
add regression coverage for this precedence.
| -- REVOKE EXECUTE on 5 internal maintenance RPCs | ||
| revoke execute on function public.correct_clinical_query_terms(text, real) from public, anon, authenticated; | ||
| revoke execute on function public.purge_expired_rag_response_cache(integer) from public, anon, authenticated; | ||
| revoke execute on function public.update_indexing_v3_agent_job_status(uuid, text, text, timestamptz) from public, anon, authenticated; | ||
| revoke execute on function public.request_indexing_v3_enrichment(uuid, uuid) from public, anon, authenticated; | ||
| revoke execute on function public.cleanup_abandoned_document_index_generations(uuid, integer, boolean) from public, anon, authenticated; | ||
| revoke execute on function public.detect_legacy_ivfflat_indexes() from public, anon, authenticated; | ||
| revoke execute on function public.document_summary_text(uuid) from public, anon, authenticated; | ||
| revoke execute on function public.search_document_chunks(uuid, text, integer, uuid) from public, anon, authenticated; | ||
| revoke execute on function public.set_document_embedding_field_content_hash() from public, anon, authenticated; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the stale RPC count.
Lines 30-38 revoke execution for nine functions, not five. Update the comment so it remains accurate for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260724163951_revoke_api_grants.sql` around lines 29 -
38, Update the comment above the revoke statements to state that execution is
being revoked for nine internal maintenance RPCs, matching the nine functions
listed below.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
scripts/test-run-lock.mjs (4)
93-97: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the standalone bypass exact.
command.includes("--standalone")treats any substrings, including--standalone-worker,--standalone-build, or unrelated textual occurrences, as a lock skip. SinceacquisitionHeavyRunLock()receives a user-constructed command string, parse the exact standalone flag or pass an explicit boolean to avoid missing lock contention for unrelated heavyweight runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-run-lock.mjs` around lines 93 - 97, Update the standalone bypass condition in acquisitionHeavyRunLock() so it recognizes only the exact --standalone command-line flag, not substrings such as --standalone-worker or textual occurrences; parse the command arguments or pass an explicit boolean while preserving the existing TEST_STANDALONE environment checks.
148-155: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not allow synchronous runners to block the heartbeat that protects the live lock.
scripts/run-vitest.mjsacquires this lock, then callsspawnSync(...). WhilespawnSyncblocks the Node.js event loop, thesetIntervalheartbeat inscripts/test-run-lock.mjscannot refreshowner.json; after 30 minutes, the current-process-holding lock is treated as stale and removed at Lines [181]-[195], allowing concurrent heavyweight commands. Move heartbeats outsidespawnSync/synchronous child runs, or update the lock time directly after waiting for the child.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-run-lock.mjs` around lines 148 - 155, Update the lock heartbeat flow in test-run-lock.mjs and the synchronous runner using spawnSync so owner.json is refreshed despite event-loop blocking. Move heartbeat execution outside the synchronous child-run path, or explicitly refresh the lock immediately after spawnSync returns, while preserving normal cleanup and stale-lock behavior.
148-155: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBind heartbeat updates to the current lock token.
In the reclaim path,
rmSync(lockPath, ...)before the next acquirer writesowner.jsonleaves the old process’s interval pointing at the same path, but it no longer updates the new owner file. Stop the interval whenreadOwner(lockPath)?.token !== token, or keep the heartbeat ownership-bound through the lock-token-controlledowner.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-run-lock.mjs` around lines 148 - 155, Update the heartbeat interval in the reclaim path to verify that readOwner(lockPath)?.token still matches the current token before calling utimesSync; when it does not, clear the interval and stop further updates. Ensure heartbeat writes remain bound to the lock owner represented by token rather than a later owner reusing the same path.
181-186: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when owner metadata cannot be read.
In
acquireHeavyRunLock(), if the owner process is alive butstatSync(path.join(lockPath, "owner.json"))throws for a transient I/O or permission error,isStaleis set totrueand the existing lock directory is removed. Treat a metadata-read failure as an acquisition error/retry, not as proof that the lock is stale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-run-lock.mjs` around lines 181 - 186, Update acquireHeavyRunLock() so failures reading owner.json metadata do not set isStale or trigger removal of the existing lock directory. Treat the statSync error as an acquisition error that follows the existing retry/error path, while retaining stale detection when mtime can be read successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/test-run-lock.mjs`:
- Around line 93-97: Update the standalone bypass condition in
acquisitionHeavyRunLock() so it recognizes only the exact --standalone
command-line flag, not substrings such as --standalone-worker or textual
occurrences; parse the command arguments or pass an explicit boolean while
preserving the existing TEST_STANDALONE environment checks.
- Around line 148-155: Update the lock heartbeat flow in test-run-lock.mjs and
the synchronous runner using spawnSync so owner.json is refreshed despite
event-loop blocking. Move heartbeat execution outside the synchronous child-run
path, or explicitly refresh the lock immediately after spawnSync returns, while
preserving normal cleanup and stale-lock behavior.
- Around line 148-155: Update the heartbeat interval in the reclaim path to
verify that readOwner(lockPath)?.token still matches the current token before
calling utimesSync; when it does not, clear the interval and stop further
updates. Ensure heartbeat writes remain bound to the lock owner represented by
token rather than a later owner reusing the same path.
- Around line 181-186: Update acquireHeavyRunLock() so failures reading
owner.json metadata do not set isStale or trigger removal of the existing lock
directory. Treat the statSync error as an acquisition error that follows the
existing retry/error path, while retaining stale detection when mtime can be
read successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 273dcde7-22c8-4c85-9ca1-559003f555b1
📒 Files selected for processing (1)
scripts/test-run-lock.mjs
|
Closing as superseded/harmless. Valuable SQL intent already landed via merged PR #1197 ( Tip-only leftovers are not desirable silent follow-ups:
PR body claims (IPv6 / PWA BroadcastChannel / etc.) are stale relative to the tip and already present on |
1 participant
Summary
RAG impact: no retrieval behaviour change — clamps non-finite/out-of-range similarity into [0,1] for scoring only; in-range scores, comparator key order, and release ranking are unchanged.
Verification
node scripts/run-vitest.mjs run tests/test-runner-safety.test.ts(17/17)node scripts/run-vitest.mjs run tests/database-skills.test.ts(4/4) andnpm run check:skillsnpm run verify:pr-localnot re-run after the latest merge; CI unit/static gates will re-validate on this head.npm run verify:uinot run in this cloud agent session.Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
PR_POLICY_BODY.mdexists so CI Sync PR policy body can write this description (agent token cannot edit the live PR body directly). Safe to delete after policy is green if you do not want the sync template retained.Summary by CodeRabbit
Search Improvements
Ingestion Updates
Security
Reliability